Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adjust Beats container user to be numeric. #41197

Merged
merged 1 commit into from
Oct 14, 2024
Merged

Adjust Beats container user to be numeric. #41197

merged 1 commit into from
Oct 14, 2024

Conversation

naemono
Copy link
Contributor

@naemono naemono commented Oct 10, 2024

Adjust Beats container user to be numeric.

"Recently" the user in the dockerfile was adjusted to be 1000 instead of metricbeat: https://github.com/elastic/beats/pull/35272/files#r1184217000

I believe this was recently changed here

See elastic/cloud-on-k8s#8086 (comment)
This is causing issues when running the container as runAsNonRoot: true:

container has runAsNonRoot and image has non-numeric user (metricbeat)

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Disruptive User Impact

This should only allow the container to run when runAsNonRoot: true is set.

Author's Checklist

  • Ensure the container builds in CI
  • Ensure e2e tests pass

How to test this PR locally

mage package I believe

Related issues

@naemono
Adjust Beats container user to be numeric.
4a7a39e 
Signed-off-by: Michael Montgomery <mmontg1@gmail.com>
@naemono naemono added the bug label Oct 10, 2024
@naemono naemono requested a review from a team as a code owner October 10, 2024 20:00
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Oct 10, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Oct 10, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @naemono? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit

@mergify Mergify
Copy link
Contributor

mergify bot commented Oct 10, 2024

backport-8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

@mergify mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Oct 10, 2024
@naemono naemono added backport-skip Skip notification from the automated backport with mergify Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team labels Oct 10, 2024
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Oct 10, 2024
@naemono naemono removed the backport-8.x Automated backport to the 8.x branch with mergify label Oct 10, 2024
@AndersonQ
Copy link
Member

@rdner you were working in the wolfi images. Is there any special reason you used non-numeric users? Otherwise I believe this PR is good to be merged

@cmacknz cmacknz requested a review from rdner October 11, 2024 21:03
@cmacknz
Copy link
Member

cmacknz commented Oct 11, 2024 •
edited
Loading

Is there any special reason you used non-numeric users?

The history is in #35272:

See
elastic/cloud-on-k8s#6688 (comment)
and
elastic/cloud-on-k8s#6740

Similar change was recently merged into elasticsearch for the same issue.

This change will allow the beats container in Kubernetes to be ran with runAsNonRoot as it checks against the numeric UID, and not a name.

Without this change you encounter the following error:

  state:                                                                                                                                                                                                                                                                                                               │
│       waiting:                                                                                                                                                                                                                                                                                                           │
│         message: 'container has runAsNonRoot and image has non-numeric user (metricbeat),                                                                                                                                                                                                                                │
│           cannot verify user is non-root (pod: "elasticsearch-es-mixed-2_beats-3064(51b07a17-26c4-4301-9380-ea6fe3e75aca)",

I'm fine with this, I do wonder if we should change the actual template input that resolves tot he .user value to be numeric to keep this from coming back (in addition to my original comment on the PR about having a test to catch this). I see similar things in the agent container template so worth cross-referencing to ensure this problem doesn't put up again with agent. https://github.com/elastic/elastic-agent/blob/0a965617920874e740b85aad7b383ea370804401/dev-tools/packaging/templates/docker/Dockerfile.elastic-agent.tmpl#L283

I'm out Monday but @rdner is back, I'll let him do the approving so someone other than me builds up context on this.

rdner
rdner approved these changes Oct 14, 2024
View reviewed changes
Copy link
Member

@rdner rdner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@naemono naemono merged commit a7915d8 into elastic:main Oct 14, 2024
142 checks passed
@naemono naemono deleted the adjust-docker-user-to-uid branch October 14, 2024 16:43
@rdner rdner added backport-8.x Automated backport to the 8.x branch with mergify backport-8.16 Automated backport with mergify and removed backport-skip Skip notification from the automated backport with mergify labels Oct 17, 2024
mergify bot pushed a commit that referenced this pull request Oct 17, 2024
@naemono @mergify
Adjust Beats container user to be numeric. (#41197)
d349d94 
Signed-off-by: Michael Montgomery <mmontg1@gmail.com>
(cherry picked from commit a7915d8)
rdner pushed a commit that referenced this pull request Oct 17, 2024
@mergify @naemono
Adjust Beats container user to be numeric. (#41197) (#41280)
01c1585 
Signed-off-by: Michael Montgomery <mmontg1@gmail.com>
(cherry picked from commit a7915d8)

Co-authored-by: Michael Montgomery <mmontg1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rdner rdner rdner approved these changes

@belimawr belimawr belimawr approved these changes

@AndersonQ AndersonQ Awaiting requested review from AndersonQ AndersonQ is a code owner automatically assigned from elastic/elastic-agent-data-plane

Assignees

@naemono naemono

Labels
backport-8.x Automated backport to the 8.x branch with mergify backport-8.16 Automated backport with mergify bug Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@naemono @elasticmachine @AndersonQ @cmacknz @belimawr @rdner